Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Deprecate ScopeManager.active() #326

Merged

Conversation

carlosalberto
Copy link
Collaborator

Addresses #267

  1. ScopeManager.active() is deprecated.
  2. Tests around it are kept, till we fully remove it.
  3. Its usage is removed from testbed though.
  4. Updated AutoFinishScopeManager (a rather exoteric manager) to also offer its capture/continue workflow without relying on a publicly visible Scope.
  5. Added a test case showing the case when the even handler or request handler has no object to store the Scope object, being solved by using a thread-local field.

Probably the important point is 5) - as it shows a concrete scenario that has historically prevented us from removing active() altogether. I'm hope this case is not over simplifying this requirement ;)

@opentracing/opentracing-java-maintainers please feel free to leave your opinion, as I'd like to have this included in 0.32 if possible ;)

@coveralls
Copy link

coveralls commented Jan 9, 2019

Coverage Status

Coverage increased (+0.9%) to 77.131% when pulling 650f48a on carlosalberto:remove_scopemanager_active into a0b972e on opentracing:v0.32.0.

Copy link

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me.

public void captureScope() throws Exception {
Span span = mock(Span.class);

// We can't use 1.7 features like try-with-resources in this repo without meddling with pom details for tests.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we consider dropping support for 1.6.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, about time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, but hopefully in a separate PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im wondering whether we should try to discuss it for 0.32 or the next iteration ;)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be on-board doing it for 0.32, but it certainly should be a separate PR and broader discussion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a discussion is appropriate but I'm not sure we should hold 0.32 for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can discuss that as part of #218 and decide what to do from there - and I agree with @austinlparker, I think we shouldn't hold 0.32 for this change to happen ;)

@yurishkuro yurishkuro changed the title Remove scopemanager active Deprecate ScopeManager.active() Jan 9, 2019
Copy link
Contributor

@sjoerdtalsma sjoerdtalsma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@carlosalberto
Copy link
Collaborator Author

Just added a commit to add the new example to the testbed README.

Unless somebody is not happy with this change, I will be merging this PR tomorrow in order to (very soon) issue a second 0.32 release candidate.

@carlosalberto carlosalberto merged commit 323f9db into opentracing:v0.32.0 Jan 16, 2019
@carlosalberto
Copy link
Collaborator Author

Merged prior to preparing a second RC for 0.32 ;)

@carlosalberto carlosalberto deleted the remove_scopemanager_active branch March 7, 2019 01:33
carlosalberto added a commit that referenced this pull request Mar 25, 2019
* Deprecate the StringTag.set() overload taking a StringTag. (#262)
* Implement Trace Identifiers. (#280)
* Bump JaCoCo to use a Java 10 friendly version (#306)
* Remove finish span on close (#301)
  * Deprecate finishSpanOnClose on activation.
  * Add ScopeManager.activeSpan() and Tracer.activateSpan().
  * Clarify the API changes and deprecations.
  * Add an error reporting sample to opentracing-testbed.
* Simple layer on top of ByteBuffer for BINARY format. (#276)
* Add generic typed setTag/withTag (#311)
* Allow injecting into maps of type Map<String,Object> (#310)
* Add simple registerIfAbsent to global tracer (#289)
* Split Inject and Extract Builtin interfaces (#316)
* Deprecate ScopeManager.active() (#326)
* Make Tracer extends Closable. (#329)
* Do not make isRegistered() synchronized. (#333)
* Deprecate AutoFinishScopeManager (#335)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants